-
Notifications
You must be signed in to change notification settings - Fork 1.4k
Fix inverted logic on registry cache expiration #9144
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
Fix inverted logic on registry cache expiration #9144
Conversation
private func unwrapRegistry(from package: PackageIdentity) throws -> (PackageIdentity.RegistryIdentity, Registry) { | ||
guard let registryIdentity = package.registry else { | ||
throw RegistryError.invalidPackageIdentity(package) | ||
} | ||
|
||
guard let registry = self.configuration.registry(for: registryIdentity.scope) else { | ||
throw RegistryError.registryNotConfigured(scope: registryIdentity.scope) | ||
} | ||
|
||
return (registryIdentity, registry) | ||
} |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
This section is just moved below to group internal scoped functions.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Thanks so much for this contribution, this is a great catch. I really appreciate the well laid out issue description as well as the test coverage.
Thanks @plemarquand! Would you be able to trigger the ci tests? |
@swift-ci test |
CI blocked by #9150 |
@swift-ci test |
1 similar comment
@swift-ci test |
@swift-ci test windows |
@plemarquand Seems like everything is passing, still waiting on a few more reviews? |
@plemarquand Apologies for the repeated pings - anything still needed on my end at this time? |
I was waiting for more eyes but honestly, LGTM so I've just merged |
Thank you @plemarquand! Definitely curious to hear more opinions, but until then hopefully we can assume no news is good news 👍 |
…9144 (#9210) Cherry pick of #9144 In `RegistryClient.swift` there is an in-memory package metadata cache expiration check ```swift let cacheKey = MetadataCacheKey(registry: registry, package: package) if let cached = self.metadataCache[cacheKey], cached.expires < .now() { return cached.metadata } ``` The logic for `cached.expires < .now()` appears to be inverted, e.g. if a cache is found for the cacheKey, the initial values look like this `2389992062152` < `2303592072235` which equates to false, because [`cached.expires`](https://github.com/swiftlang/swift-package-manager/blob/18bc4801d1e0aa02ca69633c3c12d5d1135b65bc/Sources/PackageRegistry/RegistryClient.swift#L443) is set with a TTL of [60*60 seconds](https://github.com/swiftlang/swift-package-manager/blob/18bc4801d1e0aa02ca69633c3c12d5d1135b65bc/Sources/PackageRegistry/RegistryClient.swift#L38) in the future. This leads to the cache only being used _after_ it has expired and may be causing downstream fingerprint TOFU issues when new package releases are published and the metadata cache is in-memory. ### Motivation: This issue was identified via the use of a private SPM registry conforming to the [registry server specification](https://docs.swift.org/swiftpm/documentation/packagemanagerdocs/registryserverspecification/). Disclaimer: This issue was also occurring in previous versions of Xcode but now that they are [sharing code with SPM](https://developer.apple.com/documentation/xcode-release-notes/xcode-26-release-notes#Swift-Package-Manager), I hope this example is still relevant. The scenario it is occurring in is as follows: 1. Have Xcode open for 1 hour or more after resolving dependencies at least once to populate the cache. 2. Release a new package version to the registry 3. Attempt to resolve the new version in Xcode 4. At this stage, my belief is that the previous release's metadata (including the checksum) from the "expired" cache is stored to disk as the fingerprint associated with the new release for TOFU purposes. - Code path: - -> [Get expected checksum for new version](https://github.com/swiftlang/swift-package-manager/blob/18bc4801d1e0aa02ca69633c3c12d5d1135b65bc/Sources/PackageRegistry/ChecksumTOFU.swift#L49) - -> [Get metadata for version](https://github.com/swiftlang/swift-package-manager/blob/18bc4801d1e0aa02ca69633c3c12d5d1135b65bc/Sources/PackageRegistry/ChecksumTOFU.swift#L111-L122) - -> [Check if we should hit the server or use cache](https://github.com/swiftlang/swift-package-manager/blob/18bc4801d1e0aa02ca69633c3c12d5d1135b65bc/Sources/PackageRegistry/RegistryClient.swift#L290-L310) - -> [Use cached metadata if it's been in memory for 1 hour](https://github.com/swiftlang/swift-package-manager/blob/18bc4801d1e0aa02ca69633c3c12d5d1135b65bc/Sources/PackageRegistry/RegistryClient.swift#L400-L403) - -> [Write cached (old) checksum to fingerprint storage for new version](https://github.com/swiftlang/swift-package-manager/blob/18bc4801d1e0aa02ca69633c3c12d5d1135b65bc/Sources/PackageRegistry/ChecksumTOFU.swift#L131-L138) 6. Attempting even `swift package resolve` via CLI at this point results the following error: - `invalid registry source archive checksum 'newchecksum', expected 'previouschecksum'` Because the checksum fingerprint is stored on disk in `~/Library/org.swift.swiftpm/security/fingerprints/`, it takes manual intervention to recover from this by purging various caches. Example of a common workaround: https://stackoverflow.com/questions/79417322/invalid-registry-source-archive-checksum-a-expected-b Related github issue: #8981 In addition to that, there are probably excessive requests being sent to various registries due to the cache not being used during the initial TTL. ### Modifications: Two main changes in this PR that should help with this: 1. Adding `version` property to the `MetadataCacheKey`, so that even if the previous metadata is in cache and hasn't expired, it won't be applied to the new version. 2. Reversing the logic for checking expiration, so that it will appropriately fetch new data upon expiration, rather than only fetching new data until expiration. In addition to that, this PR also: - Fixes a similar cache expiration issue for the `availabilityCache` - Adds tests for both caches - Updates `withAvailabilityCheck` to internal (from private) for testability. ### Result: The hopeful result of this is that package registry consumers will no longer require manual workarounds to fix their local state when they hit this edge case, as well as avoid hitting github and other package registries [too often](https://x.com/krzyzanowskim/status/1967333807318777900) (my guess for the original intent of the cache). That said, it will be a bit tricky to validate in its entirety because this cache is only relevant for long-running tasks via libSwiftPM I presume. One big thing to note is that this may be a noticeable change to existing behavior since the cache is not currently being used as far as I can tell, and once fixed, it will significantly reduce the freshness of the metadata compared to before. Co-authored-by: Zach Nagengast <[email protected]>
In
RegistryClient.swift
there is an in-memory package metadata cache expiration checkThe logic for
cached.expires < .now()
appears to be inverted, e.g. if a cache is found for the cacheKey, the initial values look like this2389992062152
<2303592072235
which equates to false, becausecached.expires
is set with a TTL of 60*60 seconds in the future. This leads to the cache only being used after it has expired and may be causing downstream fingerprint TOFU issues when new package releases are published and the metadata cache is in-memory.Motivation:
This issue was identified via the use of a private SPM registry conforming to the registry server specification. Disclaimer: This issue was also occurring in previous versions of Xcode but now that they are sharing code with SPM, I hope this example is still relevant. The scenario it is occurring in is as follows:
swift package resolve
via CLI at this point results the following error:invalid registry source archive checksum 'newchecksum', expected 'previouschecksum'
Because the checksum fingerprint is stored on disk in
~/Library/org.swift.swiftpm/security/fingerprints/
, it takes manual intervention to recover from this by purging various caches.Example of a common workaround: https://stackoverflow.com/questions/79417322/invalid-registry-source-archive-checksum-a-expected-b
Related github issue: #8981
In addition to that, there are probably excessive requests being sent to various registries due to the cache not being used during the initial TTL.
Modifications:
Two main changes in this PR that should help with this:
version
property to theMetadataCacheKey
, so that even if the previous metadata is in cache and hasn't expired, it won't be applied to the new version.In addition to that, this PR also:
availabilityCache
withAvailabilityCheck
to internal (from private) for testability.Result:
The hopeful result of this is that package registry consumers will no longer require manual workarounds to fix their local state when they hit this edge case, as well as avoid hitting github and other package registries too often (my guess for the original intent of the cache).
That said, it will be a bit tricky to validate in its entirety because this cache is only relevant for long-running tasks via libSwiftPM I presume.
One big thing to note is that this may be a noticeable change to existing behavior since the cache is not currently being used as far as I can tell, and once fixed, it will significantly reduce the freshness of the metadata compared to before.
Curious to hear the maintainers' thoughts on this.